A few issues here -

1. You are trying to split a bytestring (the result of the any2utf8 call) by '_' - this will not work in python3+ because literal strings are unicode by default. You've faced similar problems previously, so I think it would be helpful to understand character encodings at a conceptual level, and the differences between string handling in python2 and 3.

2. Simply sentences = utils.any2utf8(u'_'.join(sentence)).split('_')) would be enough - no need for the extra [w for w in ...]

3. We're not accounting for the possibility that a word in the sentence contains '_' here - it would be wrong to make implicit assumptions like these about user input, unless there was an explicit constraint in the API. Escaping could be an option - although I'm not sure it is feasible, performance-wise.

This should all be in the optimized path (C / Cython), so there's no point wasting time playing with Python calls.

The utf8 conversion overhead will be ~0, once this is optimized properly.

You can check with cython -a to see how much slow (Python) code is still left on the critical path. There should be none.

What happens if sentence is empty?

What happens if the sentence tokens contain ;?

How about

By default, the input sentences will be internally encoded to UTF-8 bytestrings, to save memory and ensure valid UTF-8. Set recode_to_utf8=False to skip this recoding step in case you don't care about memory or if your sentences are already bytestrings. This will result in much faster training (~2x faster).

Remove these two checks.

Comment hard to understand, and I think it's because the logic is not clear. What is it that is bytes in is_bytes?

Isn't it better to simply create a flag of whether the input sequences are bytes or not?

Then the comment becomes # do the input sentences consist of bytestrings? which is clear.

Also, this check seems to belong to learn_vocab, not here.

EDIT - ok, I think self.is_input_bytes should be ok

Adding this comment (and I think it's rightly in init) -

self.is_bytes = True  # do the input sentences consist of bytestrings?

# With default (recode_to_utf8=True) we encode input sentences to utf8 bytestrings, but
# with recode_to_utf8=False, we retain encoding, so need to store this encoding
# information to later convert token inputs accordingly (in __getitem__ and export_phrases)

If it's rightly in init, then what happens if sentences=None and user is calling learn_vocab manually later?

@piskvorky updated the PR

Best to render code blocks as literal text (put recode_to_utf8=False in backticks).

Missing full stop at the end of sentence.

Looks like a bug -- recode is False but recoding happens.

@piskvorky is it true that only unicode string is accepted (and not bytestrings) as input token in getitem or export_phrases as mentioned here in phrases.py

@piskvorky this conversion was apparently meant to handle the mismatch between bytestrings training input and unicode token input . Is this an unprecedented use case, should we just raise a warning (like TypeError for differently encoded training and token input)

Sorry, I don't understand. If the user said "I don't want recoding", we shouldn't be recoding.

What is the motivation for this?

yes, we are not doing recoding while training Phrases.

For example, for recode_to_utf8=False and for unicode input sentences (for training), we have unicode words in vocab. Now if we provide bytestrings tokens in getitem or export_phrases, this mismatch will be a problem here in phrases.py. (or vice-versa)

For, no recoding at all, I think user will have to use same encoded corpus for both training, and phrases retrieval (getitem / export_phrases)

@jayantj @gojomo @menshikh-iv ref - above comment

cc @piskvorky

I think we shouldn't be recoding implicitly in __getitem__. The only gain is in the case when learn_vocab receives bytestrings, recode_to_utf8 is False and __getitem__ receives unicode. I don't think that justifies the dangerous subtle errors that it could cause.

Also, if we're going to be implicitly handling different input formats for learn_vocab vs __getitem__, we're also going to have to take care of the delimiter here

I think delimiter issue is sorted in learn_vocab here.

Just for information, what could be those dangerous errors ? The idea is simply to do the conversion for incoming token in __getitem__ to match the encoding in vocab.

As @piskvorky mentioned, suppose we had recode_to_utf8=False, and learn vocab was called with latin2 bytestrings, and latin2 bytestrings are sent to __getitem__. We'd silently recode the latin2 bytestrings to utf8 in __getitem__ and lookup would fail, even though it should succeed.

ok, yes I understand. I think fail explicitly should be the correct choice then (for mismatch in training input and infer input for recode_to_utf8=False). What kind of error should we raise then for this mismatch ?

Looks like a bug -- recode is False but recoding happens.

Looks like a bug -- recode is False but recoding happens.

If I understand correctly, this will raise an exception if sentences is either an empty list or an empty generator.

In that case, ideally a simple test should be catching this too.

Better convert the streamed iterable to an in-memory list (using list()), it's small enough. That way we don't have to iterate over the file from disk every time.

This will make the benchmark conclusions stronger (less noise and delays from other, unrelated parts of the code, IO overhead etc).

-1: why should empty input suddenly become a special case, raising an exception?

this concern was raised here in the review comment above, and also as discussed on gitter, sentences= [ ] is not None but it will throw an error with iter in above case

Yes, but that's a reason to fix the bug, not change the API :)

oh, I should have just logged a warning Empty sentences provided as input in except block and no need to raise the error.

Unless it's really a special case (and I don't think it is), there's no need to treat it in a special way.

No exception, no warning -- there's nothing special to an empty corpus with regard to Phrases, except your new check for its first element. It's not a special case.

Using catch-all except blocks is generally a bad idea, since you could end up catching unexpected exceptions. So catch only the specific exception expected here (StopIteration?)

Debug message here would serve well too.